refactor: improve CLI value parsing/docs by 2bndy5 · Pull Request #208 · cpp-linter/cpp-linter-rs

2bndy5 · 2025-11-04T03:48:50Z

A follow-up to #204

Allow boolean options to be treated as flags (no value passed) if their default value is false (--file-changes-only, --step-summary, --*-review, etc).
Some other CLI options can now accept no value (like a flag) including --verbosity
Use std::str::FromStr trait to parse the given --version:
- if no value is given, then behave exactly like the cpp-linter version subcommand (print version and exit)
- if a path is given look for clang-tools in that path. This only ensures the given path exists, not if the clang- tool is present; that is done later if the tool is needed.
- expand support for version specifiers using semver::VersionReq. This removes the need for lenient_semver dependency and allows the user to specify a range of clang versions. For example, >=14, <16, =12.0.1, or simply 16 (would be treated as =16). See semver::VersionReq docs for more detail.
adjust docs to better describe accepted/possible values.

Due to the changes in parsing the user-given --version, there is a new enum, ClangTool that enforces type-safety about finding the clang tools (when they are needed).

Instead of passing the tool name (as a str), the ClangTool enum is used to avoid typos and convey explicit support for only clang-tidy and clang-format.
Getting the clang tool's path and version are now instance methods of the ClangTool enum.

Summary by CodeRabbit

Refactor
- Unified clang tool discovery and a flexible CLI version option that accepts a path, system default, semantic requirement, or a special “no value”; several CLI flags now accept optional values with sensible defaults.
Documentation
- CLI help now includes possible values and aliases for arguments (including explicit boolean mappings).
Chores
- Removed an unused dependency.

coderabbitai · 2025-11-04T03:57:40Z

Walkthrough

Replaces string-based version handling with a new RequestedVersion enum, introduces a public ClangTool enum and executable-resolution API, removes the lenient_semver dependency, updates CLI option parsing to support optional values, and enhances CLI documentation generation for possible values.

Changes

Cohort / File(s)	Summary
Dependency Management `cpp-linter/Cargo.toml`	Removed `lenient_semver = "0.4.2"` from `[dependencies]`.
Clang tools API `cpp-linter/src/clang_tools/mod.rs`, `cpp-linter/src/clang_tools/clang_tidy.rs`	Added public `ClangTool` enum (`ClangTidy`, `ClangFormat`) with `as_str`, `get_exe_path(&RequestedVersion)`, and `capture_version` methods. Replaced free-function `get_clang_tool_exe` usage with `ClangTool` API; updated signatures and tests to use `RequestedVersion`.
CLI parsing & types `cpp-linter/src/cli/structs.rs`, `cpp-linter/src/cli/mod.rs`	Introduced public `RequestedVersion` enum (`Path`, `SystemDefault`, `Requirement`, `NoValue`) with `FromStr`. Changed `GeneralOptions.version` type to `RequestedVersion`. Replaced `ValueEnum` derives for `LinesChangedOnly` and `ThreadComments` with manual `ValueEnum` impls. Added `default_missing_value`/`0..=1` arity adjustments for multiple CLI flags (verbosity, files_changed_only, style, tidy_checks, thread_comments, and various feedback flags).
Runtime integration & tests `cpp-linter/src/run.rs`, `cpp-linter/src/clang_tools/*` (tests)	Updated run entrypoint to treat `RequestedVersion::NoValue` as a version path, pass `&RequestedVersion` into clang-tool capture functions, removed NO-VERSION error path, and adjusted tests/flag names/usages to match new CLI semantics and ClangTool API.
Docs generation `docs/src/lib.rs`	Enhanced CLI doc generator to emit a "Possible values" section for arguments when present, including boolean mappings and enumerated values with aliases and per-value help text.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Files/areas needing extra attention:
- cpp-linter/src/clang_tools/mod.rs — verify version-resolution logic and error paths around RequestedVersion.
- cpp-linter/src/cli/structs.rs — confirm RequestedVersion::FromStr parsing (VersionReq vs path canonicalization vs magic token).
- CLI parsing changes in cpp-linter/src/cli/mod.rs and integration in src/run.rs — ensure new default_missing_value and arity semantics behave as intended.
- Updated tests under cpp-linter/src/clang_tools/* — ensure they reflect public API and edge cases.

Possibly related PRs

fix: clang-tidy diagnostic comments in PR review #77 — Edits to clang_tools/mod.rs and related capture/output logic; directly overlaps with the ClangTool/version-resolution changes.
fix: propagate errors #47 — Prior updates to get_clang_tool_exe and tool lookup behavior; related to replacing that function with ClangTool::get_exe_path.
feat: capture and output clang tool's version number #54 — Implements/relocates clang tool version capture and lookup; likely shares motivation and code paths with these changes.

Suggested labels

dependencies

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: refactoring CLI value parsing and documentation improvements, which aligns with the PR's core objectives of enhancing CLI option handling and updating documentation.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch review-cli

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1984565 and 3608b72.

📒 Files selected for processing (2)

cpp-linter/src/clang_tools/mod.rs (6 hunks)
cpp-linter/src/cli/structs.rs (6 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.842Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/clang_tools/mod.rs:149-159
Timestamp: 2024-11-23T14:10:31.760Z
Learning: Clang tools' `--version` output is consistent across all released versions, so adding extra error handling or unit tests for version extraction may be unnecessary. Tests are run after the clang tools are installed.

📚 Learning: 2025-11-04T06:50:10.842Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.842Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/cli/structs.rs

📚 Learning: 2024-11-23T06:20:11.698Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.

Applied to files:

cpp-linter/src/clang_tools/mod.rs
cpp-linter/src/cli/structs.rs

📚 Learning: 2024-10-02T07:55:08.948Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 52
File: cpp-linter/src/clang_tools/mod.rs:146-146
Timestamp: 2024-10-02T07:55:08.948Z
Learning: In the `cpp-linter` project, the function `capture_clang_tools_output` is only called in `cpp-linter/src/run.rs`, and there are no other calls to this function elsewhere in the codebase.

Applied to files:

cpp-linter/src/clang_tools/mod.rs

📚 Learning: 2024-11-23T14:10:31.760Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/clang_tools/mod.rs:149-159
Timestamp: 2024-11-23T14:10:31.760Z
Learning: Clang tools' `--version` output is consistent across all released versions, so adding extra error handling or unit tests for version extraction may be unnecessary. Tests are run after the clang tools are installed.

Applied to files:

cpp-linter/src/clang_tools/mod.rs

📚 Learning: 2025-01-21T09:56:32.771Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 101
File: cpp-linter/src/clang_tools/clang_format.rs:155-161
Timestamp: 2025-01-21T09:56:32.771Z
Learning: In cpp-linter-rs, the XML output being parsed is generated programmatically by clang-format tool. The only failure case for XML parsing is when clang-format produces a blank XML document, in which case falling back to empty results (using unwrap_or) is the desired behavior.

Applied to files:

cpp-linter/src/clang_tools/mod.rs

🧬 Code graph analysis (2)

cpp-linter/src/clang_tools/mod.rs (1)

cpp-linter/src/cli/structs.rs (5)

from_str (37-70)

from_str (116-127)

from_str (190-201)

from (234-257)

from (275-286)

cpp-linter/src/cli/structs.rs (1)

cpp-linter/src/common_fs/mod.rs (3)

normalize_path (237-262)

from (56-70)

new (44-53)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

GitHub Check: s390x-unknown-linux-gnu
GitHub Check: aarch64-unknown-linux-musl
GitHub Check: x86_64-unknown-linux-musl
GitHub Check: arm-unknown-linux-gnueabi
GitHub Check: arm-unknown-linux-gnueabihf
GitHub Check: armv7-unknown-linux-gnueabihf
GitHub Check: x86_64-unknown-linux-gnu
GitHub Check: powerpc-unknown-linux-gnu
GitHub Check: aarch64-unknown-linux-gnu
GitHub Check: test (ubuntu-latest)
GitHub Check: Build armv7-unknown-linux-gnueabihf
GitHub Check: Build x86_64-pc-windows-msvc
GitHub Check: Build aarch64-apple-darwin
GitHub Check: Build x86_64-unknown-linux-musl
GitHub Check: Build current binary
GitHub Check: Build x86_64-apple-darwin
GitHub Check: Build aarch64-unknown-linux-gnu
GitHub Check: Build aarch64-pc-windows-msvc
GitHub Check: Build x86_64-unknown-linux-gnu
GitHub Check: Build FreeBSD

🔇 Additional comments (9)

cpp-linter/src/cli/structs.rs (4)

34-71: LGTM! Path validation correctly implemented in CLI parsing.

The FromStr implementation properly handles all cases:

Empty string → SystemDefault

Sentinel value → NoValue

Version requirements (with flexible parsing for bare versions like "16" → "=16")

Paths (with existence check, parent directory extraction for files, and canonicalization)

The path handling ensures that RequestedVersion::Path always contains a directory, not a file path, which aligns with how it's used in get_exe_path. This maintains the proper separation of concerns as per the architectural principle learned.

Based on learnings.

85-128: LGTM! Well-designed ValueEnum implementation with helpful defaults.

The manual implementation provides excellent UX improvements:

Clear display values with help text for CLI --help output

Aliases for flexibility ("true"/"on"/"1", "false"/"off"/"0")

Case-insensitive parsing with sensible default fallback to Off

The default fallback ensures the parser never fails, which is appropriate for this use case.

162-202: LGTM! Consistent implementation with good UX.

Follows the same pattern as LinesChangedOnly with appropriate display values, aliases, and case-insensitive parsing with default fallback.

354-367: LGTM! Good test coverage for path validation.

The test verifies two key behaviors:

File paths have their parent directory extracted and canonicalized

Non-existent paths return an error

This confirms the from_str implementation works as designed.

cpp-linter/src/clang_tools/mod.rs (5)

32-51: LGTM! Clean type-safe abstraction for clang tools.

The ClangTool enum provides type safety by replacing string-based tool name passing with explicit variants. The Display implementation and as_str() method provide convenient string access when needed.

60-115: LGTM! Version requirement logic correctly implemented.

The method properly handles all RequestedVersion variants:

Path variant: Uses which_in to search in the specified directory. Since RequestedVersion::from_str ensures the Path variant always contains a directory (extracting parent for file paths), this works correctly.

SystemDefault/NoValue: Falls back to system PATH lookup.

Requirement variant: The version matching logic is now correct (past review concern addressed):

Finds the highest major version from all comparators using n.major > highest_major (line 80)

Decrements through versions, checking which satisfy the requirement

Searches for versioned binaries (e.g., clang-format-15) before falling back to unversioned

For example, with ">=14, <16": starts at 17, checks 15 and 14 (both match), searches for clang-format-15 and clang-format-14.

Based on learnings.

117-128: LGTM! Clean version extraction implementation.

The method runs --version and extracts the version number using a regex pattern. Per the learnings, clang tools' version output is consistent, so the straightforward approach is appropriate.

Based on learnings.

201-261: LGTM! Clean integration of the new ClangTool API.

The function has been successfully updated to use:

ClangTool::{ClangTidy,ClangFormat}.get_exe_path(&RequestedVersion) for executable lookup

ClangTool::capture_version for version extraction

The signature change from version: &str to version: &RequestedVersion provides better type safety and enables the flexible version handling (path, requirement, or default).

489-548: LGTM! Comprehensive test coverage for the new API.

The tests cover all RequestedVersion variants:

Version requirement (">=9, <22") - exercises the corrected logic

System default (empty string)

Path-based lookup

Error case (non-existent path)

The tests confirm the implementation works correctly across all scenarios.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

A follow-up to #204 - Allow boolean options to be treated as flags if their default value is false. - Some other CLI options can now accept no value (like a flag) including `--verbosity` - Use `std::str::FromStr` trait to parse the given version: * if no value is given, then behave exactly like the `cpp-linter version` subcommand (print version and exit) * if a path is given look for clang-tools in that path. This only ensures the given path exists, not if the clang- tool is present; that is done later if the tool is needed. * expand support for version specifiers using `semver::VersionReq`. This removes the need for `lenient_semver` dependency and allows the user to specify a range of clang versions. For example, `>=14, <16`, `=12.0.1`, or simply `16` (would be treated as `=16`). See [semver::VersionReq docs][ver-req-docs] for more detail. - adjust docs to better describe accepted/possible values. Due to the changes in parsing the user-given version, there is a new enum, `ClangTool` that enforces type-safety about finding the clang tools (when they are needed). * Instead of passing the tool name (as a str), the `ClangTool` enum is used to avoid typos and convey explicit support for only clang-tidy and clang-format. * Getting the clang tool's path and version are now instance methods of the `ClangTool` enum. [ver-req-docs]: https://docs.rs/semver/1.0.27/semver/struct.VersionReq.html

codecov · 2025-11-04T04:17:20Z

Codecov Report

❌ Patch coverage is 95.45455% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.53%. Comparing base (106a0e9) to head (3608b72).
⚠️ Report is 38 commits behind head on main.

Files with missing lines	Patch %	Lines
cpp-linter/src/cli/structs.rs	95.19%	5 Missing ⚠️
cpp-linter/src/clang_tools/mod.rs	94.73%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   96.61%   96.53%   -0.08%     
==========================================
  Files          14       14              
  Lines        2891     2999     +108     
==========================================
+ Hits         2793     2895     +102     
- Misses         98      104       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cpp-linter/src/clang_tools/mod.rs

* make version as path absolute * return path to exe of highest clang-tool version per requirements

fix logic op and convert path to abs dir

2bndy5 added the enhancement New feature or request label Nov 4, 2025

This comment was marked as resolved.

Sign in to view

2bndy5 changed the title ~~style: follow up to #204~~ refactor: improve CLI value parsing/docs Nov 4, 2025

2bndy5 added the documentation Improvements or additions to documentation label Nov 4, 2025

2bndy5 force-pushed the review-cli branch from 6a0edce to 387b8d0 Compare November 4, 2025 04:08

This comment was marked as resolved.

Sign in to view

2bndy5 commented Nov 4, 2025

View reviewed changes

cpp-linter/src/clang_tools/mod.rs Show resolved Hide resolved

review changes

1984565

* make version as path absolute * return path to exe of highest clang-tool version per requirements

This comment was marked as resolved.

Sign in to view

more review changes

3608b72

fix logic op and convert path to abs dir

2bndy5 merged commit b15dcfe into main Nov 4, 2025
66 checks passed

2bndy5 deleted the review-cli branch November 4, 2025 18:18

coderabbitai bot mentioned this pull request Dec 7, 2025

feat: use tokio::Command to capture suggestions #224

Closed

coderabbitai bot mentioned this pull request Jan 7, 2026

style: start phasing out .unwrap() calls #242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: improve CLI value parsing/docs#208

refactor: improve CLI value parsing/docs#208
2bndy5 merged 3 commits intomainfrom
review-cli

2bndy5 commented Nov 4, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

codecov bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

Uh oh!

Conversation

2bndy5 commented Nov 4, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Pre-merge checks and finishing touches

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

codecov bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

2bndy5 commented Nov 4, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 4, 2025 •

edited

Loading

codecov bot commented Nov 4, 2025 •

edited

Loading